Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthrough등록 화면의 상태·인텐트 구조를 재설계하고 제네릭 멤버 선택용 Composable Changes
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant Screen as PartyCreateScreen
participant BottomSheet as MemberSelectBottomSheet
participant VM as PartyCreateViewModel
User->>Screen: 멤버 선택 요청 (버튼 클릭)
Screen->>BottomSheet: showMemberBottomSheet=true, allMembers, selectedMembers 전달
BottomSheet->>User: 멤버 목록 표시
User->>BottomSheet: 멤버 선택/해제 (onMemberClick)
BottomSheet->>VM: OnMemberSelect(MemberPriceOption) 호출
VM->>VM: tempSelectedMembers 업데이트
User->>BottomSheet: main/sub 버튼 클릭
BottomSheet->>VM: OnMemberSelectDone / OnCloseBottomSheet 호출
VM->>Screen: uiState 업데이트 (showMemberBottomSheet=false, selectedMembers 반영)
Screen->>User: 선택 반영하여 UI 갱신
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/component/CreateMemberSetting.kt`:
- Line 91: There is a typo in the MemberSettingStatus enum: rename the enum
constant ARTITST_NOT_SELECTED to ARTIST_NOT_SELECTED in the MemberSettingStatus
declaration (Contracts.kt) and update all references (e.g., usages in
CreateMemberSetting.kt such as the branch handling
MemberSettingStatus.ARTITST_NOT_SELECTED at the two places flagged) to the
corrected symbol ARTIST_NOT_SELECTED so the enum name and all matchings compile.
In
`@app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt`:
- Around line 18-22: The enum MemberSettingStatus contains a typo in the value
ARTITST_NOT_SELECTED; rename it to ARTIST_NOT_SELECTED and update every usage
site that references MemberSettingStatus.ARTITST_NOT_SELECTED (search for
ARTITST_NOT_SELECTED or usages of MemberSettingStatus) to the new identifier,
and run a build/IDE refactor to ensure all references (switches, comparisons,
serialization, parcelables, tests) are updated consistently to avoid
regressions.
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt`:
- Around line 70-71: Replace the unstable LaunchedEffect key
uiState.isInitialized with the stable navigation parameters so the
initialization intent isn't retriggered; specifically, change
LaunchedEffect(uiState.isInitialized) to LaunchedEffect(artistId, artistName,
productName) and keep the viewModel.processIntent(InitializeScreen(artistId,
artistName, productName)) call (symbols: LaunchedEffect, uiState.isInitialized,
viewModel.processIntent, InitializeScreen, artistId, artistName, productName).
- Around line 80-88: The UI is performing business logic by reading
uiState.imageUris and calling toImageInfosForPresigned inside the ConvertUris
handler (risking stale closure and violating UDF); instead, stop calling
toImageInfosForPresigned from PartyCreateScreen and either (A) include the
current image list as payload on the ConvertUris intent/effect so the ViewModel
can run conversion, or (B) move the conversion into a ViewModel-level use case
and handle ConvertUris entirely in the ViewModel (emit ConvertDone from there).
Update the UI to only dispatch the ConvertUris intent (or dispatch with the
imageUris payload) and remove any direct calls to toImageInfosForPresigned and
viewModel.processIntent(ConvertDone(...)) from the composable; reference
symbols: ConvertUris, uiState.imageUris, toImageInfosForPresigned, ConvertDone,
viewModel.processIntent.
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt`:
- Line 199: There is a misspelled function name searchProdut; rename the
function declaration and all its usages to searchProduct in PartyCreateViewModel
(e.g., update the method definition named searchProdut and the calls like
searchProdut(keyword) to searchProduct(keyword)), and ensure any imports,
references or tests that call searchProdut are updated accordingly so the
project compiles.
🧹 Nitpick comments (5)
app/src/main/java/com/poti/android/presentation/party/component/MemberSelectBottomSheet.kt (3)
48-50:remember키에memberToId람다가 포함되지 않았습니다.
memberToId람다가 변경되어도selectedMemberIds가 재계산되지 않아 stale 데이터 문제가 발생할 수 있습니다. 다만, 실제 사용 시memberToId가 안정적인 함수 참조라면 문제가 없을 수 있습니다.♻️ 개선 제안
- val selectedMemberIds = remember(selectedMembers) { + val selectedMemberIds = remember(selectedMembers, memberToId) { selectedMembers.map { memberToId(it) }.toSet() }
30-44: 외부에서 주입받을Modifier파라미터가 누락되었습니다.코딩 가이드라인에 따르면
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 합니다. 현재 구현에서는Modifier가 노출되지 않아 호출부에서 레이아웃 커스터마이징이 제한됩니다.♻️ Modifier 파라미터 추가 제안
fun <T> MemberSelectBottomSheet( title: String, mainBtnText: String, subBtnText: String, onDismiss: () -> Unit, onMainBtnClick: () -> Unit, onSubBtnClick: () -> Unit, allMembers: List<T>, selectedMembers: List<T>, onMemberClick: (T) -> Unit, memberToName: (T) -> String, memberToId: (T) -> Long, mainEnabled: Boolean, subEnabled: Boolean, autoCloseSubBtn: Boolean = true, + modifier: Modifier = Modifier, ) {As per coding guidelines:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 해.
110-114: Preview 데이터가 실제 제네릭 타입과 일치하지 않습니다.
allMembers에listOf("원영, 유진")을 전달하고,memberToName이 빈 문자열을 반환하며,memberToId가 항상0L을 반환합니다. 이로 인해 Preview에서 실제 동작을 확인하기 어렵습니다.♻️ Preview 데이터 개선 제안
MemberSelectBottomSheet( title = stringResource(R.string.action_button_continue), onDismiss = {}, mainBtnText = stringResource(R.string.action_button_continue), onMainBtnClick = {}, mainEnabled = true, subBtnText = stringResource(R.string.action_button_continue), onSubBtnClick = {}, subEnabled = true, - allMembers = listOf("원영, 유진"), - selectedMembers = listOf("원영"), + allMembers = listOf("원영", "유진", "레이"), + selectedMembers = listOf("원영"), onMemberClick = {}, - memberToName = { "" }, - memberToId = { 0L }, + memberToName = { it }, + memberToId = { it.hashCode().toLong() }, )app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt (1)
41-41:imageUris에ImmutableList<Uri>사용을 고려해주세요.현재
List<Uri>는 Compose 컴파일러가 불안정(unstable)으로 간주하여 불필요한 리컴포지션을 유발할 수 있습니다. 다른 필드들(rawMembers,selectedDeliveries등)처럼ImmutableList를 사용하면 안정성이 보장됩니다.♻️ ImmutableList 사용 제안
data class CreateUiState( - val imageUris: List<Uri> = emptyList(), + val imageUris: ImmutableList<Uri> = persistentListOf(), val imageError: FieldError? = null,그리고
PartyCreateViewModel에서 업데이트 시:- imageUris = intent.uris.toPersistentList(), + imageUris = intent.uris.toPersistentList(),(이미
toPersistentList()사용 중이라면 타입만 변경하면 됩니다)As per coding guidelines:
List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해@immutable이나@stable어노테이션 사용을 제안해줘.app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt (1)
65-77: 여러updateState호출을 하나로 병합하는 것을 권장합니다.
OnBack과OnBackConfirm처리에서updateState를 여러 번 호출하고 있습니다. 이는 불필요한 상태 emissions을 유발할 수 있습니다. 하나의copy()호출로 병합하면 성능이 개선됩니다.♻️ updateState 병합 제안
OnBack -> if (uiState.value.isFieldTouched) { updateState { copy(showDialog = true) } } else { - updateState { copy(showDialog = false) } - sendEffect(NavigateToBack) - updateState { copy(isInitialized = false) } + updateState { copy(showDialog = false, isInitialized = false) } + sendEffect(NavigateToBack) } OnBackConfirm -> { - updateState { copy(showDialog = false) } - sendEffect(NavigateToBack) - updateState { copy(isInitialized = false) } + updateState { copy(showDialog = false, isInitialized = false) } + sendEffect(NavigateToBack) }
| NavigateToBack -> { | ||
| onPopBackStack() | ||
| } | ||
|
|
||
| CreateUiEffect.NavigateToSearch -> onNavigateToSearch() | ||
|
|
||
| CreateUiEffect.ShowBottomSheet -> { | ||
| showBottomSheet = true | ||
| } | ||
|
|
||
| CreateUiEffect.ShowDialog -> { | ||
| showDialog = true | ||
| } | ||
| NavigateToSearch -> onNavigateToSearch() | ||
|
|
||
| CreateUiEffect.ConvertUris -> { | ||
| val result = uiState.selectedImages.toImageInfosForPresigned(context) | ||
| viewModel.processIntent(CreateUiIntent.OnConvertDone(result)) | ||
| ConvertUris -> { | ||
| val result = uiState.imageUris.toImageInfosForPresigned(context) | ||
| viewModel.processIntent(ConvertDone(result)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PartyCreateScreen.kt" | head -5Repository: team-poti/POTI-ANDROID
Length of output: 149
🏁 Script executed:
rg -l "HandleSideEffects|ConvertUris" --type kt | head -10Repository: team-poti/POTI-ANDROID
Length of output: 92
🏁 Script executed:
fd "PartyCreateScreen.kt" -type fRepository: team-poti/POTI-ANDROID
Length of output: 236
🏁 Script executed:
wc -l ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.ktRepository: team-poti/POTI-ANDROID
Length of output: 153
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt | sed -n '60,110p'Repository: team-poti/POTI-ANDROID
Length of output: 2302
🏁 Script executed:
rg "fun HandleSideEffects" --max-count 5 -A 10Repository: team-poti/POTI-ANDROID
Length of output: 48
🏁 Script executed:
rg "sealed.*class.*PartyCreateSideEffect|object.*ConvertUris" --max-count 10Repository: team-poti/POTI-ANDROID
Length of output: 190
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt | head -40Repository: team-poti/POTI-ANDROID
Length of output: 2441
🏁 Script executed:
find . -type f -name "*.kt" | xargs grep -l "fun HandleSideEffects" | head -1Repository: team-poti/POTI-ANDROID
Length of output: 48
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/core/common/util/HandleSideEffects.ktRepository: team-poti/POTI-ANDROID
Length of output: 1002
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt | head -80Repository: team-poti/POTI-ANDROID
Length of output: 4367
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt | sed -n '80,150p'Repository: team-poti/POTI-ANDROID
Length of output: 2597
🏁 Script executed:
find . -type f -name "PartyCreateViewModel.kt" -type fRepository: team-poti/POTI-ANDROID
Length of output: 152
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt | head -100Repository: team-poti/POTI-ANDROID
Length of output: 5294
🏁 Script executed:
rg "ConvertUris" ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt -A 5 -B 5Repository: team-poti/POTI-ANDROID
Length of output: 433
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt | sed -n '100,200p'Repository: team-poti/POTI-ANDROID
Length of output: 4196
🏁 Script executed:
rg "OnCreateClick|ConvertDone" ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt -B 3 -A 10Repository: team-poti/POTI-ANDROID
Length of output: 823
🏁 Script executed:
rg "fun validateInputs" ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt -A 20Repository: team-poti/POTI-ANDROID
Length of output: 1384
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt | sed -n '65,95p'Repository: team-poti/POTI-ANDROID
Length of output: 1145
🏁 Script executed:
rg "rememberUpdatedState" . --type-list -lRepository: team-poti/POTI-ANDROID
Length of output: 455
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt | sed -n '1,30p'Repository: team-poti/POTI-ANDROID
Length of output: 1648
🏁 Script executed:
rg "rememberUpdatedState" . -lRepository: team-poti/POTI-ANDROID
Length of output: 48
🏁 Script executed:
python3 << 'EOF'
# Analyze the flow of ConvertUris effect
print("=== ConvertUris Effect Flow Analysis ===\n")
print("1. USER TRIGGERS:")
print(" OnCreateClick intent sent\n")
print("2. VIEWMODEL PROCESSES:")
print(" - validateInputs() checks imageUris (line 158)")
print(" - If validation passes: sendEffect(ConvertUris)")
print(" - Effect goes into sideEffect Flow\n")
print("3. UI RECEIVES EFFECT:")
print(" - HandleSideEffects collects effect")
print(" - Lambda executes: uiState.imageUris.toImageInfosForPresigned(context)")
print(" - Sends ConvertDone intent back to ViewModel\n")
print("CLOSURE CAPTURE CONCERN:")
print(" The lambda { effect -> ... } is captured in LaunchedEffect")
print(" LaunchedEffect dependencies: sideEffectFlow, lifecycleOwner")
print(" If uiState changes, recomposition occurs but LaunchedEffect")
print(" doesn't re-enter (keys unchanged), so lambda uses OLD uiState\n")
print("TIMING VULNERABILITY:")
print(" If user changes images while ConvertUris effect is in flight,")
print(" the effect handler would use OLD imageUris from closure\n")
print("EXISTING ARCHITECTURE:")
print(" ✓ ConvertUris is CreateUiEffect (sealed interface)")
print(" ✓ Already separated from UiState")
print(" ✓ Using SharedFlow/Flow pattern correctly")
print(" ✗ But closure in HandleSideEffects can capture stale state")
EOFRepository: team-poti/POTI-ANDROID
Length of output: 1150
🏁 Script executed:
rg "HandleSideEffects" . -A 8 -B 2 | head -60Repository: team-poti/POTI-ANDROID
Length of output: 7003
🏁 Script executed:
python3 << 'EOF'
# Check if ConvertUris really needs access to latest uiState
print("=== Architectural Analysis ===\n")
print("CURRENT FLOW:")
print("1. UI receives ConvertUris effect")
print("2. UI reads uiState.imageUris from closure")
print("3. UI transforms URIs to ImageInfoForPresigned")
print("4. UI sends ConvertDone(result) back to ViewModel")
print("5. ViewModel receives ConvertDone and proceeds\n")
print("PROBLEM:")
print("- Transformation logic (toImageInfosForPresigned) lives in UI")
print("- This violates MVI principle: UI shouldn't perform business logic")
print("- Closure captures stale state\n")
print("BETTER APPROACH:")
print("1. ConvertUris payload contains imageUris data")
print("2. ViewModel receives data with effect payload")
print("3. ViewModel performs transformation")
print("4. ViewModel sends NavigateToUpload or similar\n")
print("BUT CURRENT CODE:")
print("- ConvertUris is data object (no payload)")
print("- UI must read from captured uiState")
print("- This is the source of the problem")
EOFRepository: team-poti/POTI-ANDROID
Length of output: 832
🏁 Script executed:
rg "toImageInfosForPresigned" . -B 3 -A 3Repository: team-poti/POTI-ANDROID
Length of output: 3325
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt | sed -n '1,20p'Repository: team-poti/POTI-ANDROID
Length of output: 1074
ConvertUris 효과 처리 시 UI 비즈니스 로직 분리 필요합니다.
현재 코드는 HandleSideEffects 람다에서 uiState.imageUris에 접근하여 변환 작업을 수행하고 있는데, 이는 MVI 아키텍처에서 UI가 비즈니스 로직을 직접 실행하는 것으로 두 가지 문제가 있습니다:
-
클로저 캡처 문제: LaunchedEffect의 의존성이
[sideEffectFlow, lifecycleOwner]이므로, 사용자가 이미지를 변경한 후 ConvertUris 효과가 처리될 때 수집된 lambda는 이전uiState값을 참조할 수 있습니다. -
아키텍처 위반: 이미지 변환 로직(
toImageInfosForPresigned)이 UI 레이어에 있어 비즈니스 로직 분리 원칙을 위배합니다.
ViewModel에서 ConvertUris 효과를 처리할 때 이미지 목록을 payload로 함께 전달하거나, 변환 로직을 ViewModel의 별도 UseCase로 이동하는 것이 권장됩니다. 그렇게 하면 상태 데이터의 최신성도 보장하고 단방향 데이터 흐름(UDF)도 명확해집니다.
🤖 Prompt for AI Agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt`
around lines 80 - 88, The UI is performing business logic by reading
uiState.imageUris and calling toImageInfosForPresigned inside the ConvertUris
handler (risking stale closure and violating UDF); instead, stop calling
toImageInfosForPresigned from PartyCreateScreen and either (A) include the
current image list as payload on the ConvertUris intent/effect so the ViewModel
can run conversion, or (B) move the conversion into a ViewModel-level use case
and handle ConvertUris entirely in the ViewModel (emit ConvertDone from there).
Update the UI to only dispatch the ConvertUris intent (or dispatch with the
imageUris payload) and remove any direct calls to toImageInfosForPresigned and
viewModel.processIntent(ConvertDone(...)) from the composable; reference
symbols: ConvertUris, uiState.imageUris, toImageInfosForPresigned, ConvertDone,
viewModel.processIntent.
There was a problem hiding this comment.
다른 브랜치에 이미지 변환로직 개편해둠! 추후 수정 예정
There was a problem hiding this comment.
@doyeon0307, 알겠습니다! 다른 브랜치에서 이미 작업 중이시군요.
이 리팩토링을 잊지 않도록 추후 작업을 위한 이슈를 생성해 드릴까요? 원하시면 이 PR과 코멘트 링크를 포함한 follow-up 이슈를 만들어드릴 수 있습니다.
( •ᴗ• )و✧
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt`:
- Around line 172-177: LaunchedEffect currently calls onScrollComplete()
unconditionally even when uiState.errorIndexToScroll is null; change the logic
so onScrollComplete() is only invoked when errorIndexToScroll is non-null (e.g.,
move the onScrollComplete() call inside the uiState.errorIndexToScroll?.let {
... } block or guard it with an if (uiState.errorIndexToScroll != null) check)
so that listState.animateScrollToItem(...) and onScrollComplete() execute
together only when there is an index to scroll to.
🧹 Nitpick comments (5)
app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt (1)
41-41:imageUris에ImmutableList<Uri>사용을 권장드려요.현재
List<Uri>를 사용하고 있는데, 다른 필드들(rawMembers,selectedMembers,rawDeliveries,selectedDeliveries)은ImmutableList를 사용하고 있어요. 일관성과 불필요한 리컴포지션 방지를 위해ImmutableList<Uri>로 변경하시는 것이 좋겠습니다.♻️ 수정 제안
data class CreateUiState( - val imageUris: List<Uri> = emptyList(), + val imageUris: ImmutableList<Uri> = persistentListOf(),app/src/main/java/com/poti/android/presentation/party/create/component/CreateMemberSetting.kt (1)
94-114: 멤버 리스트가 커질 경우LazyColumn사용을 고려해 주세요.현재
Column+forEachIndexed로 구현되어 있는데, 멤버 수가 많아지면 성능 이슈가 발생할 수 있어요. 만약 멤버 수가 일정 수준 이상으로 늘어날 가능성이 있다면LazyColumn과key파라미터 사용을 권장드립니다.현재 구조가 의도적이라면 무시하셔도 됩니다.
app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt (2)
65-77:sendEffect후updateState순서에 주의가 필요해요.
sendEffect(NavigateToBack)호출 후updateState { copy(isInitialized = false) }를 호출하고 있는데, 네비게이션이 즉시 발생하면 상태 업데이트가 적용되기 전에 화면이 전환될 수 있어요.isInitialized = false상태가 다음 화면 진입 시 필요하다면, 상태 업데이트를 먼저 수행하는 것이 안전합니다.♻️ 수정 제안
OnBack -> if (uiState.value.isFieldTouched) { updateState { copy(showDialog = true) } } else { - updateState { copy(showDialog = false) } - sendEffect(NavigateToBack) updateState { copy(isInitialized = false) } + sendEffect(NavigateToBack) } OnBackConfirm -> { - updateState { copy(showDialog = false) } - sendEffect(NavigateToBack) updateState { copy(isInitialized = false) } + sendEffect(NavigateToBack) }
490-500:artistId ?: 0L대신 명시적인 예외 처리를 권장드려요.
validateInputs()에서selectedArtist가 null인 경우를 체크하지만, 방어적 코딩 관점에서0L대신 명시적인 에러 처리가 더 안전해요. 예상치 못한 경우에 잘못된 데이터가 서버로 전송되는 것을 방지할 수 있습니다.♻️ 수정 제안
private suspend fun uploadPartyInfo(urls: List<String>): Result<Long> = createPartyUseCase( - artistId = uiState.value.selectedArtist?.artistId ?: 0L, + artistId = uiState.value.selectedArtist?.artistId + ?: error("selectedArtist should not be null at this point"),app/src/main/java/com/poti/android/presentation/party/create/PartyCreateScreen.kt (1)
193-196:LazyColumn아이템에key파라미터 추가를 고려해 주세요.현재
animateItem을 사용하고 있는데, 명시적인key가 없으면 아이템 재정렬 시 애니메이션이 예상대로 동작하지 않을 수 있어요. 각 섹션에 고유한key를 지정하면 성능과 애니메이션 동작이 개선됩니다.♻️ 수정 제안 예시
LazyColumn( state = listState, modifier = Modifier.padding(innerPadding), ) { - item { + item(key = "product_info") { // ... } - item { + item(key = "artist") { // ... } // ... 나머지 아이템들도 동일하게 key 추가 }
| text = title, | ||
| modifier = Modifier | ||
| .padding(top = 12.dp, bottom = 16.dp, start = screenWidthDp(16.dp)) | ||
| .background(PotiTheme.colors.white), |
There was a problem hiding this comment.
P2: 텍스트에는 따로 background 안 줘도 될 것 같아여
| LazyVerticalGrid( | ||
| columns = GridCells.Fixed(2), | ||
| modifier = Modifier | ||
| .height(492.dp) |
There was a problem hiding this comment.
P2: 고정 높이 안 쓰고 screenHeight 사용해주면 화면 비율에 맞출 수 있을 것 같습니당
There was a problem hiding this comment.
P2: 이렇게 값을 하나하나 넘겨줄 수도 있지만 price 값만 바뀌었기 때문에 copy를 사용하면 훨씬 간단합니다. 나중에 MemberPriceOption의 다른 필드가 바뀌었을 때도 수정할 필요가 없습니다
val newOption = option.copy(price = newPrice)| ) | ||
|
|
||
| if (showHint && isEditBtnInScreen && !isKeyboardVisible) { | ||
| if (!hideHint && showHint && isEditBtnInScreen && !isKeyboardVisible) { |
There was a problem hiding this comment.
P1: showHint와 hideHint의 역할이 다소 중복되는 것 같습니다.
이 힌트는 초기 진입 시 한 번 노출되고 인터랙션 발생 시 사라지는 일회성 UI이므로, 굳이 UiState에서 상태를 주입받기보다 내부 상태만으로 관리하는 것이 더 간단하고 명확할 것 같은데, showHint 파라미터를 제거하고 내부 로직으로만 처리하는 방식은 어떠신가요???
There was a problem hiding this comment.
헐 맞아요 제가 넘 복잡하게 생각했어요
툴팁은 단순히 editButton과 함께 보여지면 되는 거라 showHint 플래그 자체가 불필요해요!!
hideHint로 터치 이벤트 발생에 따라 버튼이 있어도 툴팁이 숨겨지는 케이스만 제어하면 됩니다
파라미터 제거 후 내부로직으로만 처리되게 수정했어욤
| showHint = false | ||
| } | ||
| }, | ||
| onFocusChanged = { _ -> hideHint = true }, |
There was a problem hiding this comment.
P1: onFocusChanged는 컴포넌트가 처음 그려질 때 초기 포커스 상태를 알리기 위해 한 번 즉시 호출됩니다. 결과적으로 EditOptionPrice의 onFocusChanged가 false로 초기화되면서 hideHint가 true로 바뀌기 때문에 툴팁 박스가 생기지 않습니다. 따라서 기존처럼 focus 되었을 때만 true로 바뀌도록 하는 것이 좋을 것 같네여
| onFocusChanged = { _ -> hideHint = true }, | |
| onFocusChanged = { isFocused -> | |
| if (isFocused) hideHint = true | |
| }, |
| .onGloballyPositioned { coordinates -> | ||
| val buttonTop = coordinates.positionInWindow().y | ||
| isEditBtnInScreen = buttonTop < screenHeight - bottomBtnHeight | ||
| }, |
There was a problem hiding this comment.
P3: 이 버튼이 화면 위로 밀려 올라갈 일은 거의 없을 것 같긴 하지만 isEditBtnInScreen를 화면 바닥 기준으로만 계산하지 않고 위로 사라졌을 경우도 함께 처리하면 좋을 것 같아욤
.onGloballyPositioned { coordinates ->
val bounds = coordinates.boundsInWindow()
val isVisible = (bounds.top < screenHeight) && (bounds.bottom > 0)
if (isEditBtnInScreen != isVisible) {
isEditBtnInScreen = isVisible
}
},There was a problem hiding this comment.
넴!
사실 isEditBtnInScreen 네이밍에 따르면 button의 bottom 기준으로 bounds.bottom > 0 조건을 거는 편이 더 정확하긴 한데욤..
이 플래그가 필요한 이유가 툴팁 표시를 위함인데, 툴팁은 버튼 상단에 표시되기 때문에 button의 top 값을 활용하는 편이 UI 적으로는 더 자연스러울 것 같아요
우선은 top y좌표만을 사용해 버튼이 화면 내 있는지 판단하도록 해봤습니다..!
There was a problem hiding this comment.
P3: configuration.screenHeightDp 대신 displayMetrics.heightPixels을 사용할 수도 있어요. configuration은 dp를 픽셀로 변환해서 사용하지만 displayMetrics은 바로 픽셀을 가져옵니당
There was a problem hiding this comment.
위 리뷰 반영하면서 screenHeight 값을 활용하지 않게 되었는데 담에 displayMetrics 적용해보겟습니다!! 감사해용
| CreateDropdownField( | ||
| viewType = ViewType.ARTSIT_SELECT, | ||
| value = uiState.artistSearchKeyword, | ||
| value = uiState.artistQuery, |
There was a problem hiding this comment.
P3: 이름을 이렇게 바꾼 이유가 있나욤?? artistSearchKeyword가 뭔가 더 직관적인 것 같은데
There was a problem hiding this comment.
artistSearchKeyword가 넘 길다 싶어서 바꿔봤는대 artistSearchKeyword로 돌아가도록 하겟습니다
There was a problem hiding this comment.
P1: 뷰모델에서 SavedStateHandle을 사용하면 값을 직접 가져올 수 있기 때문에 굳이 Route를 통해 값을 넘겨주지 않아도 됩니당. 그럼 UiIntent에 InitializeScreen을 사용할 필요도 없을 것 같아요
There was a problem hiding this comment.
제가 그래프 개념을 몰랐어가지고.. 중첩그래프 설정없이 sharedViewModel을 쓰다보니 등록뷰에서 나가더라도 생명주기가 연동되지 않아 뷰모델이 유지되는 문제가 있었어요.
그래서 Route가 직접 파라미터를 받는 어색한 로직이 있었는데
이 리뷰 보고 뷰모델 초기화 방식을 더 알아보고 PartyCreateGraph를 추가해 화면을 나가면 뷰모델도 사라지도록 했습니다. >Route의 초기화 코드를 포함해 수동 초기화 로직을 모두 제거 + savedStateHandle을 적용했어요
| is CreateUiIntent.OnBackConfirm -> { | ||
| updateState { CreateUiState() } | ||
| sendEffect(CreateUiEffect.NavigateToBack) | ||
| OnBack -> if (uiState.value.isFieldTouched) { |
There was a problem hiding this comment.
P1: isFieldTouched보다는 값이 채워진 필드가 있는지 여부를 확인해서 다이얼로그를 띄워주는게 나을 것 같은데 어떻게 생각하시나요?
값이 바뀔 때 isFieldTouched를 직접 true로 바꿔주면 누락되는 경우가 있을 수도 있어요. 지금 코드에서도 handleMemberSelectDone과 handleMemberPriceChange에서 isFieldTouched를 true로 바꿔주지 않아서 편집 후 나갈 때 다이얼로그가 뜨지 않습니다. 또한 사용자가 글을 썼다 지워서 필드가 비어 있는 경우에 isFieldTouched가 true가 되어 내용이 작성되어 있지 않아도 다이얼로그가 뜨게 돼요.
따라서 UiState에 현재 값이 비어있지 않은지 검사하는 프로퍼티를 만들어서 사용하는 것이 좋을 것 같습니다. imageUris나 description, deadline 등을 isNotEmpty와 isNotBlack를 통해 계산하여 함수에 isFieldTouched 값을 바꿔주지 않아도 자동으로 계산된 값을 사용하면 실수할 일이 없을 것 같아요
There was a problem hiding this comment.
넴!!! 필드값으로 다이얼로그 띄우는데 완전 동의하빈다
UiState에서 computed 프로퍼티 만들어서 해보려고 햇는데 등록뷰가 조금 복잡하게 설계되어있어서 뷰모델 내에 shouldShowDialog() 메소드 만들어 처리해보았어요.
이유는 굿즈별 페이지에서 진입 시 파라미터로 ArtistId/ArtistName/ProductName을 전달받아 필드를 채우게 되는데, 이 경우는 사용자가 직접 입력한 경우가 아니어서 다이얼로그를 안 띄우는게 맞다는 생각이 들었어요
따라서 뷰모델이 들고 있는 파라미터값과 현재 필드의 Artist/Product 값이 동일하다면 다이얼로그 뜨지 않도록 했습니다
- as-is : screenHeight, bottomButtonHeight 활용 - to-be : layoutBottom 활용
- artistQuery to artistSearchKeyword
- as-is : Route에서 수동 초기화 - to-be : PartyCreate 중첩 그래프 추가로 Create 뷰와 뷰모델 생명주기 연동 > 자동 초기화
- as-is : 필드 입력 한 번이라도 발생 시 true - to-be : 각 필드 입력값과 초기값 일치하는지 비교
|
✅ Ktlint check passed. Run: https://github.com/team-poti/POTI-ANDROID/actions/runs/21742599775 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/poti/android/presentation/party/create/PartyArtistSelectScreen.kt (1)
93-97:⚠️ Potential issue | 🟡 Minor
ViewType.ARTSIT_SELECT의 오타를 수정해야 합니다.
ARTSIT_SELECT는ARTIST_SELECT로 수정되어야 합니다. 현재 enum 정의(CreateDropdownField.kt, 49번 줄)와 사용처 2곳에서 모두 이 오타가 일관되게 반복되고 있습니다.현재 코드
enum class ViewType( val maxHeight: Dp, ) { CREATE_PARTY(156.dp), ARTSIT_SELECT(500.dp), // ← 오타 }이번 리팩토링 기회에 enum 정의와 함께 사용하는 2곳을 모두 수정하시면 좋겠습니다.
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/component/CreateMemberSetting.kt`:
- Around line 162-173: In the preview data passed to CreateMemberSetting, the
MemberPriceOption instances all use the same memberId (memberId = 1); update the
sample data so each MemberPriceOption has a unique memberId (e.g., 1, 2, 3) to
reflect real-world usage and avoid confusion when rendering the composable in
CreateMemberSetting and when inspecting MemberPriceOption entries in previews.
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt`:
- Around line 54-56: The KtLint indentation error is caused by the misaligned
parameter in the PartyCreateViewModel constructor call to BaseViewModel; locate
PartyCreateViewModel and the BaseViewModel(...) invocation and fix the
indentation of the parameter `initialState = CreateUiState()` so it follows
KtLint rules (align parameters consistently with the opening parenthesis or
place the parameter on the same line as the constructor call), ensuring the
`initialState = CreateUiState()` line is indented to match other parameters or
the opening parenthesis of BaseViewModel.
- Around line 406-421: handleMemberPriceChange currently calls hasInvalidPrice()
which reads uiState.value.selectedMembers and thus sees the old state; compute
validation against the newly built newMembers instead and use that result when
updating state. Specifically, in handleMemberPriceChange (and before calling
updateState { ... }), run the invalid-price check on newMembers (or create a
hasInvalidPriceFor(members) helper) and then set memberError = null when that
check is false inside the updateState copy, ensuring selectedMembers =
newMembers and memberError are derived from the same newMembers data.
🧹 Nitpick comments (4)
app/src/main/java/com/poti/android/presentation/party/component/MemberSelectBottomSheet.kt (2)
31-46: 제네릭List<T>파라미터의 안정성(Stability) 고려가 필요합니다.
allMembers: List<T>와selectedMembers: List<T>는 Compose 컴파일러가 불안정(unstable)하다고 판단하여 불필요한 리컴포지션을 유발할 수 있습니다. 현재 호출부(PartyCreateScreen)에서ImmutableList를 전달하고 있으므로 실질적 문제는 적지만, 제네릭 컴포넌트이므로 다른 호출부에서의 오용 가능성이 있습니다.파라미터 타입을
ImmutableList<T>로 변경하시거나, 최소한 KDoc으로 안정적인 컬렉션을 전달해야 한다는 점을 명시하시면 좋겠습니다.As per coding guidelines, "List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘."
64-77: 패딩 적용 방식이 일관되지 않습니다.Line 67에서
title의start패딩은screenWidthDp(16.dp)를 사용하고 있지만, Line 77의contentPadding에서는start = 16.dp,end = 16.dp로 raw 값을 직접 사용하고 있습니다. 기기 대응을 위해 동일한 유틸리티를 일관되게 적용하시는 것이 좋겠습니다.♻️ 패딩 일관성 수정 제안
LazyVerticalGrid( columns = GridCells.Fixed(2), modifier = Modifier .height(screenHeightDp(492.dp)) .fillMaxWidth(), - contentPadding = PaddingValues(start = 16.dp, end = 16.dp, top = 12.dp, bottom = 40.dp), + contentPadding = PaddingValues(start = screenWidthDp(16.dp), end = screenWidthDp(16.dp), top = 12.dp, bottom = 40.dp), horizontalArrangement = Arrangement.spacedBy(12.dp), verticalArrangement = Arrangement.spacedBy(12.dp), ) {As per coding guidelines, "하드코딩된 값(색상, 크기, 문자열) 대신
Theme,Dimens,strings.xml리소스를 사용하는지 체크해줘."app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt (1)
40-78:imageUris: List<Uri>가 다른 리스트 필드와 달리ImmutableList를 사용하지 않고 있습니다.
rawMembers,selectedMembers,rawDeliveries,selectedDeliveries등은ImmutableList를 사용하고 있는 반면,imageUris는List<Uri>로 선언되어 있습니다. Compose 컴파일러 관점에서List는 불안정(unstable)으로 간주되어 불필요한 리컴포지션을 유발할 수 있습니다. 일관성을 위해ImmutableList<Uri>로 변경하시는 것을 권장드립니다.♻️ 수정 제안
data class CreateUiState( - val imageUris: List<Uri> = emptyList(), + val imageUris: ImmutableList<Uri> = persistentListOf(), val imageError: FieldError? = null,As per coding guidelines, "List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘."app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt (1)
445-493:validateInputs()에러 검증과 스크롤 인덱스 매핑이 체계적입니다. 👍모든 필드에 대한 검증을 일괄 수행하고, 첫 번째 에러 필드로 자동 스크롤하는 구조가 사용자 경험 관점에서 좋습니다.
FieldErrorenum과@StringRes를 활용한 에러 메시지 관리도 타입 안전합니다.다만,
getScrollIndex()의 하드코딩된 인덱스(0-7)가LazyColumn의item순서와 정확히 일치하는지 유지보수 시 주의가 필요합니다. 향후 아이템 순서 변경 시 동기화가 누락될 수 있으므로, 상수나 enum으로 인덱스를 관리하시는 것도 고려해 보시면 좋겠습니다.
- as-is : 새 입력값을 검사해야 하는데, 기존값을 검사해 에러상태 업데이트 - to-be : 새 입력값으로 에러 검사
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt`:
- Around line 417-426: The guard in handleDeliverySelect uses the 'in' operator
(full-object equals) while the toggle logic compares deliveryId, causing
inconsistent behavior; change the guard to check membership by ID (e.g.,
currentDeliveries.any { it.deliveryId == newDelivery.deliveryId }) so both the
early-return and the toggle use the same deliveryId-based comparison and the
"don't remove the last selected delivery" check works correctly.
- Around line 120-125: In PartyCreateViewModel's OnAccountNumberChange handling,
the accountNumberError is cleared based on the raw intent.value instead of the
digits-only filtered value; change the logic so you compute the filtered value
(the result of intent.value.filter { it.isDigit() }) into a local variable and
use that variable (or the new accountNumber) when deciding accountNumberError =
if (filtered.isNotBlank()) null else this.accountNumberError so errors are only
cleared when the stored accountNumber is non-empty.
🧹 Nitpick comments (3)
app/src/main/java/com/poti/android/presentation/party/create/model/Contracts.kt (2)
41-41:imageUris타입을ImmutableList<Uri>로 변경하는 것을 권장드립니다.현재
List<Uri>로 선언되어 있으나, ViewModel에서는toPersistentList()로 할당하고 계시고, 동일UiState내 다른 컬렉션 프로퍼티(rawMembers,selectedMembers,rawDeliveries등)는ImmutableList<T>를 사용하고 계십니다. Compose 컴파일러는List를 불안정(unstable) 타입으로 판단하여 불필요한 리컴포지션을 유발할 수 있습니다. 일관성과 안정성을 위해ImmutableList<Uri>로 맞춰주시면 좋겠습니다.♻️ 수정 제안
data class CreateUiState( - val imageUris: List<Uri> = emptyList(), + val imageUris: ImmutableList<Uri> = persistentListOf(), val imageError: FieldError? = null,As per coding guidelines, "안정성(Stability): List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘."
127-129:ConvertDone은 사용자 인텐트보다 내부 콜백에 가까워 보입니다.
ConvertDone은 이미지 변환 완료 후 ViewModel로 결과를 전달하는 내부 이벤트입니다.UiIntent에 포함하면 MVI의 "사용자 의도" 개념이 희석될 수 있으므로, 추후 리팩토링 시 별도 내부 채널이나 콜백 메커니즘으로 분리하는 것을 고려해 보시면 좋겠습니다.app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt (1)
503-513:selectedArtist?.artistId ?: 0L의 무음 폴백이 우려됩니다.
validateInputs()에서selectedArtist != null을 보장하므로 정상 플로우에서는 문제없지만, 방어적으로0L을 전송하면 서버에 잘못된 데이터가 생성될 수 있습니다. 검증 후 도달하는 코드이므로!!또는 명시적 에러 처리가 더 안전합니다.♻️ 수정 제안
- artistId = uiState.value.selectedArtist?.artistId ?: 0L, + artistId = uiState.value.selectedArtist?.artistId + ?: error("selectedArtist must not be null after validation"),
| is OnAccountNumberChange -> updateState { | ||
| copy( | ||
| accountNumber = intent.value.filter { it.isDigit() }, | ||
| accountNumberError = if (intent.value.isNotBlank()) null else this.accountNumberError, | ||
| ) | ||
| } |
There was a problem hiding this comment.
accountNumberError 클리어 조건이 필터링된 값 기준이 아닙니다.
accountNumber는 intent.value.filter { it.isDigit() }로 숫자만 저장하지만, 에러 클리어는 intent.value.isNotBlank() (원본 입력값) 기준입니다. 예를 들어 사용자가 문자만 입력하면 ("abc"), intent.value는 비어있지 않아 에러가 클리어되지만, 실제 accountNumber는 ""이 됩니다.
🐛 수정 제안
is OnAccountNumberChange -> updateState {
+ val filtered = intent.value.filter { it.isDigit() }
copy(
- accountNumber = intent.value.filter { it.isDigit() },
- accountNumberError = if (intent.value.isNotBlank()) null else this.accountNumberError,
+ accountNumber = filtered,
+ accountNumberError = if (filtered.isNotBlank()) null else this.accountNumberError,
)
}🤖 Prompt for AI Agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt`
around lines 120 - 125, In PartyCreateViewModel's OnAccountNumberChange
handling, the accountNumberError is cleared based on the raw intent.value
instead of the digits-only filtered value; change the logic so you compute the
filtered value (the result of intent.value.filter { it.isDigit() }) into a local
variable and use that variable (or the new accountNumber) when deciding
accountNumberError = if (filtered.isNotBlank()) null else
this.accountNumberError so errors are only cleared when the stored accountNumber
is non-empty.
| private fun handleDeliverySelect(newDelivery: DeliveryOption) { | ||
| val currentDeliveries = uiState.value.selectedDeliveries | ||
|
|
||
| updateState { | ||
| copy( | ||
| sheetDisplayMemberIndices = newIndices, | ||
| isSheetTouched = true, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private fun handleMemberSelectDone() { | ||
| val newIds = uiState.value.editableMemberOptions.mapIndexedNotNull { index, option -> | ||
| if (index in uiState.value.sheetDisplayMemberIndices) { | ||
| option.memberId | ||
| } else { | ||
| null | ||
| } | ||
| }.toSet() | ||
| if (currentDeliveries.size < 2 && newDelivery in currentDeliveries) return | ||
|
|
||
| val newMemberOptions = clearUnselectedOptionPrices(newIds) | ||
| val newDeliveries = if (currentDeliveries.any { it.deliveryId == newDelivery.deliveryId }) { | ||
| currentDeliveries.filter { it.deliveryId != newDelivery.deliveryId } | ||
| } else { | ||
| currentDeliveries + newDelivery | ||
| }.toPersistentList() |
There was a problem hiding this comment.
handleDeliverySelect에서 비교 방식이 일관되지 않습니다.
Line 420의 가드 조건은 in 연산자(전체 필드 equals() 비교)를 사용하고, Line 422의 토글 로직은 deliveryId만 비교합니다. DeliveryOption 객체가 동일 ID이지만 다른 필드를 가지는 경우, 가드를 우회하여 배송 옵션이 0개가 될 수 있습니다. 비교 방식을 통일해 주시면 좋겠습니다.
🐛 수정 제안
private fun handleDeliverySelect(newDelivery: DeliveryOption) {
val currentDeliveries = uiState.value.selectedDeliveries
- if (currentDeliveries.size < 2 && newDelivery in currentDeliveries) return
+ if (currentDeliveries.size < 2 && currentDeliveries.any { it.deliveryId == newDelivery.deliveryId }) return
val newDeliveries = if (currentDeliveries.any { it.deliveryId == newDelivery.deliveryId }) {🤖 Prompt for AI Agents
In
`@app/src/main/java/com/poti/android/presentation/party/create/PartyCreateViewModel.kt`
around lines 417 - 426, The guard in handleDeliverySelect uses the 'in' operator
(full-object equals) while the toggle logic compares deliveryId, causing
inconsistent behavior; change the guard to check membership by ID (e.g.,
currentDeliveries.any { it.deliveryId == newDelivery.deliveryId }) so both the
early-return and the toggle use the same deliveryId-based comparison and the
"don't remove the last selected delivery" check works correctly.
There was a problem hiding this comment.
기존에 screenHeight를 사용할 때 기준으로 얘기했는데 지금 방식도 괜찮은 것 같네욤👍🏻👍🏻
| selectedDeliveryIds = this.selectedDeliveryIds + result.first().deliveryId, | ||
| deliveriesState = ApiState.Success(deliveries), | ||
| rawDeliveries = deliveries, | ||
| selectedDeliveries = deliveries, |
There was a problem hiding this comment.
P3: 근데 맨 처음에 모든 딜리버리 옵션이 다 선택되도록 했었나욤???
There was a problem hiding this comment.
요거 기본값이 전체선택임니다 !!
cmj7271
left a comment
cmj7271
left a comment
There was a problem hiding this comment.
깔끔한 코드 덕분에 이 정도 PR 이야 거뜬하군요
수고했슨
3 participants
Related issue 🛠️
Work Description ✏️
등록 뷰 코드를 전면적으로 수정했어요
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
죄송함니다 마지막 폐급PR입니다 기존 코드가 폐급뚱뚱코드였기때문에 리팩토링하며 다시 폐급PR을 올릴수박에 없엇음니다
Summary by CodeRabbit
릴리스 노트
New Features
Bug Fixes
Refactor
Chores